Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiCollapsibleNavBeta] Fix bug with long/scrolling pages #8141

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Nov 14, 2024

Summary

Addresses elastic/kibana#198870

Before After
before after

QA

  • yarn build-pack and link to local Kibana + test that this fixes the issue

General checklist

N/A, beta component

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen cee-chen enabled auto-merge (squash) November 14, 2024 22:15
@sebelga
Copy link
Contributor

sebelga commented Nov 18, 2024

Thanks for opening this PR!
Do we agree that #8139 still relevant as it is a better UX to close the popover when clicking on a link?

@sebelga
Copy link
Contributor

sebelga commented Nov 18, 2024

Maybe another approach to fix this is to close the popover when the window scrolls?

In the gif above the repositioning does not look completely smooth at the end.

@cee-chen
Copy link
Member Author

Do we agree that #8139 still relevant as it is a better UX to close the popover when clicking on a link?

Yes, still relevant IMO - we should have both :)

@cee-chen
Copy link
Member Author

cee-chen commented Nov 18, 2024

Maybe another approach to fix this is to close the popover when the window scrolls?

Yeah I spiked out trying to absolutely position the popover to the fixed nav (rather than portalled to the body) but sadly our popover positioning calculation isn't built for that 🥲 I'd rather just leave this as-is for simplicity. It's a bit of an edge case in any case, it doesn't need to be perfect, and a one-liner fix is fine IMO.

@cee-chen cee-chen merged commit 4e66c43 into elastic:main Nov 19, 2024
8 of 9 checks passed
@cee-chen cee-chen deleted the nav-beta-fix branch November 19, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants